Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces packing/unpacking functionality for directories and adds a read-only archive-based cache implementation. However, the code contains several critical bugs that prevent compilation and correct execution.
Key changes:
- Added
internal/packpackage with ZIP compression and decompression functions - Added
internal/caches/archivepackage for read-only ZIP-backed cache - Updated documentation in
internal/caches/localto clarify cache compatibility
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| internal/pack/pack.go | New package providing Pack/Unpack functions for ZIP compression of directories, with resource leak issue and commented-out code |
| internal/caches/local/local.go | Documentation improvements clarifying compatibility with built-in cache |
| internal/caches/archive/archive.go | New read-only ZIP-backed cache implementation with multiple critical bugs (incomplete struct field, uninitialized fields, unused function call, missing return value) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| type Cache struct { | ||
| zr *zip.Reader | ||
| c io.Closer | ||
| d |
There was a problem hiding this comment.
Incomplete field declaration. The field d has no type specified, which will cause a compilation error.
|
|
||
| // Get implements [cache.Cache]. | ||
| func (c *Cache) Get(id cache.ActionID) (cache.Entry, error) { | ||
| return c.dc.Get(id) |
There was a problem hiding this comment.
Undefined field access. The field c.dc is accessed but never initialized in the New or Open functions, which will cause a nil pointer dereference at runtime.
| dst.Close() | ||
| if err != nil { | ||
| resErr = lazyerrors.Error(err) | ||
| return | ||
| } |
There was a problem hiding this comment.
Resource leak on error path. If f.Open() succeeds but an error is returned, the dst file handle is closed but its error is ignored. Consider checking the close error or using a deferred close with proper error handling.
| dst.Close() | |
| if err != nil { | |
| resErr = lazyerrors.Error(err) | |
| return | |
| } | |
| closeErr := dst.Close() | |
| if err != nil { | |
| resErr = lazyerrors.Error(err) | |
| return | |
| } | |
| if closeErr != nil && resErr == nil { | |
| resErr = lazyerrors.Error(closeErr) | |
| return | |
| } |
| }, nil | ||
| } | ||
|
|
||
| func Open(file string, l *slog.Logger) (*Cache, error) { |
There was a problem hiding this comment.
[nitpick] Function naming inconsistency. The function Open should be named consistently with Go conventions, such as OpenFile or NewFromFile, to clearly indicate it opens a cache from a file path. Additionally, it should have a documentation comment explaining its purpose.
| func Open(file string, l *slog.Logger) (*Cache, error) { | |
| // OpenFile opens a read-only Cache backed by a zip archive at the given file path. | |
| func OpenFile(file string, l *slog.Logger) (*Cache, error) { |
| l: l, | ||
| }, nil | ||
| } | ||
|
|
There was a problem hiding this comment.
Missing documentation. The exported function Open lacks a documentation comment, which is required for all exported identifiers in Go.
| // Open opens a zip archive file and returns a new [Cache]. |
| return nil, lazyerrors.Error(err) | ||
| } | ||
|
|
||
| os.CreateTemp() |
There was a problem hiding this comment.
Unused function call. os.CreateTemp() is called but its return values are ignored and the created temporary file is never used or cleaned up. This will create a resource leak.
| os.CreateTemp() |
|
|
||
| // OutputFile implements [cache.Cache]. | ||
| func (c *Cache) OutputFile(id cache.OutputID) string { | ||
| return c.dc.OutputFile(id) |
There was a problem hiding this comment.
Undefined field access. The field c.dc is accessed but never initialized in the New or Open functions, which will cause a nil pointer dereference at runtime.
|
|
||
| // FuzzDir implements [cache.Cache]. | ||
| func (c *Cache) FuzzDir() string { | ||
| return |
There was a problem hiding this comment.
Empty return value. The function should return a string representing the fuzz directory, but returns an empty value instead.
| return | |
| return "" |
| // err = root.MkdirAll(fp[:len(fp)-len(f.FileInfo().Name())], 0o755) | ||
| // if err != nil { | ||
| // resErr = lazyerrors.Error(err) | ||
| // return | ||
| // } | ||
|
|
There was a problem hiding this comment.
Commented-out code should be removed. This block of code for creating parent directories appears to be incomplete work that should either be implemented or removed before merging.
| // err = root.MkdirAll(fp[:len(fp)-len(f.FileInfo().Name())], 0o755) | |
| // if err != nil { | |
| // resErr = lazyerrors.Error(err) | |
| // return | |
| // } |
1 participant
No description provided.